Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

v1:Respect the shim_debug flag when load tasks #3276

Merged
merged 1 commit into from
May 13, 2019
Merged

v1:Respect the shim_debug flag when load tasks #3276

merged 1 commit into from
May 13, 2019

Conversation

darfux
Copy link
Contributor

@darfux darfux commented May 13, 2019

Currently when we restart containerd it will load all tasks with shim
logs whether the shim_debug is set or not.

Signed-off-by: Li Yuxuan liyuxuan04@baidu.com

@darfux darfux force-pushed the v1_respect_shim_debug branch 3 times, most recently from a3910bb to a30a190 Compare May 13, 2019 04:31
@codecov-io
Copy link

codecov-io commented May 13, 2019 •
edited

Codecov Report

Merging #3276 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #3276   +/-   ##
======================================
  Coverage    44.4%   44.4%           
======================================
  Files         113     113           
  Lines       12231   12231           
======================================
  Hits         5431    5431           
  Misses       5966    5966           
  Partials      834     834
Flag Coverage Δ
#linux 48.33% <ø> (ø) ⬆️
#windows 39.66% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c0d0fc3...66036d9. Read the comment docs.

fuweid
fuweid approved these changes May 13, 2019
View reviewed changes
Copy link
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@crosbymichael
Copy link
Member

I think we may want to forward to /dev/null if debug is not set so that we make sure the pipes are flushed and don't backup and block the shim process

@darfux
Copy link
Contributor Author

darfux commented May 13, 2019

I think we may want to forward to /dev/null if debug is not set so that we make sure the pipes are flushed and don't backup and block the shim process

@crosbymichael Great idea! So should we also forward to /dev/null when the shim process starting ?

containerd/runtime/v1/shim/client/client.go

Lines 70 to 84 in 0485499

if debug {
stdoutLog, err = v1.OpenShimStdoutLog(ctx, config.WorkDir)
if err != nil {
return nil, nil, errors.Wrapf(err, "failed to create stdout log")
}
stderrLog, err = v1.OpenShimStderrLog(ctx, config.WorkDir)
if err != nil {
return nil, nil, errors.Wrapf(err, "failed to create stderr log")
}
go io.Copy(os.Stdout, stdoutLog)
go io.Copy(os.Stderr, stderrLog)
}

@crosbymichael
Copy link
Member

@darfux ya, lets go ahead and make that change as well

@darfux
v1: Respect the shim_debug flag when load tasks
66036d9 
Currently when we restart containerd it will load all tasks with shim
logs whether the `shim_debug` is set or not.

Signed-off-by: Li Yuxuan <liyuxuan04@baidu.com>
@theopenlab-ci
Copy link

theopenlab-ci bot commented May 13, 2019

Build succeeded.

@darfux
Copy link
Contributor Author

darfux commented May 13, 2019

@crosbymichael I found that the stdout&stderr of shim will be set to nil when starting if debug is not set. So I just forward to /dev/null when load tasks. :D

@crosbymichael
Copy link
Member

LGTM

@crosbymichael crosbymichael merged commit 7acdb16 into containerd:master May 13, 2019
@crosbymichael
Copy link
Member

thanks @darfux

@darfux darfux deleted the v1_respect_shim_debug branch May 14, 2019 01:56
@darfux darfux restored the v1_respect_shim_debug branch August 8, 2019 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fuweid fuweid fuweid approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@darfux @codecov-io @crosbymichael @fuweid